v6 orderbook #427
Open
v6 orderbook #427
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds V6 support: config/subgraph entries, new ABIs (orderbook/deployer V6), versioned subgraph handling, order/orderbook version fields, V5/V6 take-orders config and calldata paths, contract resolution for v6, and corresponding tests across simulators, routers, and state. Changes
Sequence Diagram(s)sequenceDiagram
participant Subgraph as SubgraphManager
participant Sync as OrderSync
participant Order as OrderManager
participant Router as Router/Simulator
participant Contracts as SolverContracts
participant Chain as Onchain/ABI
Subgraph->>Sync: fetchAll(urls) with per-URL version
Note right of Subgraph: results annotated with __version
Sync->>Order: iterEvents yields events + version
Order->>Order: attach event.order.__version
Order->>Router: build Pair (includes orderbookVersion)
Router->>Contracts: getAddressesForTrade(orderbookVersion)
Router->>Router: select getTakeOrdersConfigV3|V4|V5 (branch by orderbookVersion)
Router->>Chain: encode calldata using selected ABI (arb3|arb4|arb5)
Chain-->>Router: calldata
Router-->>Order: return encoded calldata/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/modes/router/simulate.ts (1)
62-62: Update documentation to reflect V6 support.The comment states the simulator "supports both V4 and V5 orderbook (order v3 and v4) types" but the implementation now also includes V6 orderbook support.
📝 Suggested documentation update
- * The simulator supports both V4 and V5 orderbook (order v3 and v4) types. + * The simulator supports V4, V5, and V6 orderbook (order v3 and v4) types.src/subgraph/index.ts (1)
146-165: Consider defaulting version insidefetchSubgraphOrders.
Since the version is derived from the URL anyway, allowing the method to resolve it when omitted reduces the chance of mismatched tagging by external callers.♻️ Suggested adjustment
- async fetchSubgraphOrders(url: string, version: SubgraphVersions): Promise<SgOrder[]> { + async fetchSubgraphOrders(url: string, version?: SubgraphVersions): Promise<SgOrder[]> { const result: SgOrder[] = []; let skip = 0; let timestamp = Date.now(); for (;;) { timestamp = Date.now(); const res = await axios.post( url, { query: getQueryPaginated(skip, this.filters), }, { headers, timeout: this.requestTimeout }, ); if (res?.data?.data?.orders) { const orders: SgOrder[] = res.data.data.orders; - if (version === SubgraphVersions.V6) { + const resolvedVersion = version ?? this.getSubgraphVersion(url); + if (resolvedVersion === SubgraphVersions.V6) { orders.forEach((v) => (v.__version = SubgraphVersions.V6)); } else { orders.forEach((v) => (v.__version = SubgraphVersions.OLD_V)); }
🤖 Fix all issues with AI agents
In `@src/common/abis/orderbook.ts`:
- Around line 103-154: The TakeOrdersConfigV5 ABI struct fields in namespace _v6
(symbol TakeOrdersConfigV5) don't match the TypeScript type
(TakeOrdersConfigTypeV5) — update the ABI string for TakeOrdersConfigV5 to use
minimumIO, maximumIO, maximumIORatio, add the bool IOIsInput field, then orders
and data (i.e. `(${Float} minimumIO, ${Float} maximumIO, ${Float}
maximumIORatio, bool IOIsInput, ${TakeOrderConfigV4}[] orders, bytes data)`) so
calldata encoding for takeOrders4 and arb5 will align with the TypeScript types.
In `@src/core/modes/inter/simulate.test.ts`:
- Around line 638-691: The test description is wrong: update the it(...)
description that currently reads "should return v4 config" to accurately say
"should return v5 config" (the test is exercising getTakeOrdersConfigV5 and
asserting V5-specific fields like minimumIO, maximumIO, maximumIORatio,
IOIsInput), i.e., locate the it block with the string literal "should return v4
config" and change it to reference V5 so the test name matches the behavior
being tested.
In `@src/core/modes/inter/simulate.ts`:
- Around line 323-350: The method getTakeOrdersConfigV5 currently has a
docstring mentioning "V4" which is incorrect; update the comment to describe V5
semantics and parameters: change the header to reference TakeOrdersConfigTypeV5
and V5 order/counterparty parameters, and adjust any parameter descriptions
(orderDetails: PairV4 -> V5-specific wording, counterpartyOrderDetails: Pair) to
reflect this method builds a V5 take-orders config including encodedFN; ensure
the summary, param descriptions, and return type mention V5
(TakeOrdersConfigTypeV5) and that data is ABI-encoded with orderbook and
encodedFN.
In `@src/order/sync.ts`:
- Around line 18-19: The yield currently sets version: res.__version
unconditionally which can overwrite an existing event.order.__version; change
the assignment to guard against undefined by using res.__version when it's
defined, otherwise fall back to the existing event.order.__version (e.g.
version: res.__version !== undefined ? res.__version : event?.order?.__version).
Update both occurrences where the yield/object uses version (the one with yield
{ event, url, timestamp: Number(res.timestamp), version: res.__version } and the
similar occurrence around lines 79-80) so you only overwrite when the upstream
provides a __version.
In `@src/order/types/v4.ts`:
- Around line 85-93: The comment above TakeOrdersConfigTypeV5 is misleading
because it says "version 6 orderbook" while the type name indicates this is the
V5 config shape; update the comment or name so they align — e.g., change the
comment to "Represents the V5 take orders configuration used by the version 6
orderbook (config schema v5)" or rename TakeOrdersConfigTypeV5 to
TakeOrdersConfigTypeV6 if the intent is a V6 config; adjust any references to
the type accordingly.
In `@src/router/types.ts`:
- Around line 235-244: The JSDoc incorrectly states "Creates a new
TakeOrdersConfigTypeV4" while the implementation constructs
TakeOrdersConfigTypeV5; update the documentation to reference
TakeOrdersConfigTypeV5 (and any other V4 mentions) so it matches the produced
type. Locate the JSDoc block documenting the method that returns
TakeOrdersConfigTypeV5 and replace "V4" with "V5", and adjust any param text if
it still incorrectly refers to v4 order objects to accurately say v5 where
applicable.
In `@src/subgraph/types.ts`:
- Around line 103-106: The enum member OLD_V in SubgraphVersions is ambiguous;
rename OLD_V to a clearer identifier such as LEGACY or PRE_V6 in the
SubgraphVersions enum declaration and update all references/usages of
SubgraphVersions.OLD_V across the codebase (imports, switch cases, comparisons,
and tests) to the new name to preserve behavior and ensure type safety.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/state/contracts.ts (2)
72-101: Consider extracting version-specific configuration to reduce duplication.The
resolveVersionContractsfunction now has repeated conditional checks forversion === "v6". While functional, this could become harder to maintain as more versions are added.Consider a lookup-based approach:
♻️ Suggested refactor for version configuration
const VERSION_CONFIG = { v4: { interpreterFn: "iInterpreter", storeFn: "iStore", abi: ABI.Deployer.Primary.Deployer }, v5: { interpreterFn: "iInterpreter", storeFn: "iStore", abi: ABI.Deployer.Primary.Deployer }, v6: { interpreterFn: "I_INTERPRETER", storeFn: "I_STORE", abi: ABI.Deployer.Primary.DeployerV6 }, } as const; // Then use: const config = VERSION_CONFIG[version ?? "v4"]; const interpreter = await client.readContract({ address: addresses.dispair, functionName: config.interpreterFn, abi: config.abi, }).catch(() => undefined);
103-109: Consider using a typed result instead ofany.Using
anyfor theresultobject bypasses TypeScript's type checking. A more type-safe approach would be to explicitly type the result.🛡️ Suggested fix for type safety
- const result: any = { + const result: NonNullable<SolverContracts["v4" | "v5" | "v6"]> = { dispair: { deployer: addresses.dispair, - interpreter, - store, + interpreter: interpreter as `0x${string}`, + store: store as `0x${string}`, }, };
🤖 Fix all issues with AI agents
In `@src/common/abis/orderbook.ts`:
- Around line 149-154: Remove or document the commented-out arb4 signature
inside the exported Arb constant: either delete the commented line referencing
`arb4(address orderBook, ${TakeOrdersConfigV5} calldata startTakeOrders,
${TakeOrdersConfigV5} calldata endTakeOrders, bytes calldata exchangeData,
${TaskV2} calldata task) external payable` to eliminate dead code, or replace it
with a brief TODO noting planned implementation and rationale (e.g., "TODO:
implement arb4 for v4 routing support — keep signature for future use") so
readers know why it remains; update the surrounding `Arb` export accordingly
(symbols: Arb, arb4, arb5, TakeOrdersConfigV5, TaskV2).
- Around line 249-260: Update the incorrect JSDoc references inside the V6
namespace to say "v6" where appropriate: change the comment for
Primary.Orderbook (currently "Orderbook v4 contract ABI") to "Orderbook v6
contract ABI", change any mentions like "Orderbook v4 structs" to "Orderbook v6
structs", and update the summary line that reads "Orderbook v4 and Arb
contracts" to "Orderbook v6 and Arb contracts"; keep the OrderStructAbi JSDoc
referencing OrderV4 unchanged since the struct name is OrderV4. Ensure you
modify comments near the V6 namespace and the Primary exports (Arb, Orderbook,
OrderStructAbi) so all JSDoc text consistently reflects v6.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config.example.yaml`:
- Around line 35-40: Update the comment above the subgraph list (the lines
describing v6 behavior) to correct "thats" to "that's" and rephrase for clarity:
state that because the v5 and v6 subgraphs use the same schema/format, you must
prefix a v6 URL with "v6=" to explicitly mark it as v6; keep the example entry
"- v6=https://v6-subgraph.com" and ensure the comment mentions the need for the
"v6=" prefix to distinguish v6 despite identical schemas.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/subgraph/index.ts (1)
58-63: Avoid returning values fromforEachcallbacks (Biome error).
Biome flags these callbacks because the assignment expression returns a value. Use block bodies (or a simplefor...of) to keep callbacksvoid.🧹 Suggested fixes
- this.subgraphs.forEach( - (url) => - (this.syncState[url] = { - skip: 0, - lastFetchTimestamp: 0, - }), - ); + this.subgraphs.forEach((url) => { + this.syncState[url] = { + skip: 0, + lastFetchTimestamp: 0, + }; + });- if (version === SubgraphVersions.V6) { - orders.forEach((v) => (v.__version = SubgraphVersions.V6)); - } else { - orders.forEach((v) => (v.__version = SubgraphVersions.LEGACY)); - } + const tag = + version === SubgraphVersions.V6 ? SubgraphVersions.V6 : SubgraphVersions.LEGACY; + orders.forEach((v) => { + v.__version = tag; + });- if (this.getSubgraphVersion(url) === SubgraphVersions.V6) { - txs.forEach((v) => (v.__version = SubgraphVersions.V6)); - } else { - txs.forEach((v) => (v.__version = SubgraphVersions.LEGACY)); - } + const tag = + this.getSubgraphVersion(url) === SubgraphVersions.V6 + ? SubgraphVersions.V6 + : SubgraphVersions.LEGACY; + txs.forEach((v) => { + v.__version = tag; + });Also applies to: 146-166, 242-248
🤖 Fix all issues with AI agents
In `@src/subgraph/index.ts`:
- Around line 19-23: The top-line comment above the SubgraphVersionList type
contains a typo ("subgraoh"); update the comment to read something like "keeps
track of subgraph versions" (or a grammatically complete sentence) so the
comment for the exported type SubgraphVersionList accurately spells "subgraph"
and clearly describes the type's purpose.
- Around line 276-283: getSubgraphVersion currently checks the exact URL against
this.versions.v6 and will misclassify config-style strings like "v6=..."; update
getSubgraphVersion to normalize the input by stripping any leading "v6=" prefix
(case-sensitive or optionally case-insensitive if you prefer) and then perform
the lookup against this.versions.v6, returning SubgraphVersions.V6 when found
and SubgraphVersions.LEGACY otherwise (refer to the getSubgraphVersion method
and this.versions.v6 symbol to locate the change).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Updates
Tests
✏️ Tip: You can customize this high-level summary in your review settings.